Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] tls: fix legacy SecurePair clienthello race window #26452

Closed
wants to merge 2 commits into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Mar 5, 2019

There is a time window between the first and the last step of processing
the clienthello event and the SecurePair may have been destroyed during
that interval.

Fixes: #26428

The first commit fixes a bug that's been around for five years.

CI: https://ci.nodejs.org/job/node-test-pull-request/21215/

This seems to have been broken ever since its introduction 5 years ago
in commit 75ea11f ("tls: introduce asynchronous `newSession`") and
no one complained but that's not going to stop me from fixing it anyway
because otherwise I can't write a regression test for issue nodejs#26428.

Refs: nodejs#26428
There is a time window between the first and the last step of processing
the clienthello event and the SecurePair may have been destroyed during
that interval.

Fixes: nodejs#26428
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. tls Issues and PRs related to the tls subsystem. v8.x labels Mar 5, 2019
sessionCb();
server.close();
}, 100);
}
Copy link
Member Author

@bnoordhuis bnoordhuis Mar 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apropos the somewhat uncommon style in this file, it's a mashup of two (fairly old and crufty) tests: test-tls-async-cb-after-socket-end.js and test-tls-securepair-server.js. I didn't want to deviate too much from the style used in those files.

setTimeout(function() {
sessionCb();
server.close();
}, 100);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need common.platformTimeout() ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not used in test-tls-async-cb-after-socket-end.js either so I'm inclined to say it's not.

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Mar 5, 2019
@BridgeAR BridgeAR changed the title tls: fix legacy SecurePair clienthello race window [8.x] tls: fix legacy SecurePair clienthello race window Mar 5, 2019
@bnoordhuis
Copy link
Member Author

CI failures on AIX and Windows appear unrelated.

@sam-github
Copy link
Contributor

BethGriggs pushed a commit that referenced this pull request Mar 21, 2019
This seems to have been broken ever since its introduction 5 years ago
in commit 75ea11f ("tls: introduce asynchronous `newSession`") and
no one complained but that's not going to stop me from fixing it anyway
because otherwise I can't write a regression test for issue #26428.

Refs: #26428

PR-URL: #26452
Fixes: #26428
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
BethGriggs pushed a commit that referenced this pull request Mar 21, 2019
There is a time window between the first and the last step of processing
the clienthello event and the SecurePair may have been destroyed during
that interval.

Fixes: #26428

PR-URL: #26452
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@BethGriggs
Copy link
Member

Landed on v8.x-staging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants